-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-18610: [ABFS] OAuth2 Token Provider support for Azure Workload Identity #6787
Conversation
...ls/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsIntegrationTest.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Hi @steveloughran @mukund-thakur |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added few comments:
...-azure/src/main/java/org/apache/hadoop/fs/azurebfs/oauth2/WorkloadIdentityTokenProvider.java
Outdated
Show resolved
Hide resolved
...-azure/src/main/java/org/apache/hadoop/fs/azurebfs/oauth2/WorkloadIdentityTokenProvider.java
Outdated
Show resolved
Hide resolved
// In case token is not refreshed for 1 hr or any clock skew issues, | ||
// refresh token. | ||
expiring = elapsedTimeSinceLastTokenRefreshInMillis >= ONE_HOUR | ||
|| elapsedTimeSinceLastTokenRefreshInMillis < 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in case of clock skew we will be renewing during every call?
I would at least add a warning message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a debug log added. Should I change it to warn then??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also same logic ported from MSI Token provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retaining the logic of refreshing tokens in case of clock skews..
🎊 +1 overall
This message was automatically generated. |
...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/oauth2/AzureADAuthenticator.java
Outdated
Show resolved
Hide resolved
...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/oauth2/AzureADAuthenticator.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
@rakeshadr @mukund-thakur Thanks. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending one minor comment
expiring = elapsedTimeSinceLastTokenRefreshInMillis >= ONE_HOUR | ||
|| elapsedTimeSinceLastTokenRefreshInMillis < 0; | ||
System.currentTimeMillis() - tokenFetchTime; | ||
boolean expiring = elapsedTimeSinceLastTokenRefreshInMillis < 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this expiring variable now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily. It is just there for a cleaner code.
If we get rid of it, we will have this replaced with elapsedTimeSinceLastTokenRefreshInMillis < 0
at two places.
Seems okay to me that way as well.
Will take this,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah..sorry somehow I missed that expiring is getting returned. I think it was better before.
Do you mind reverting this part?
so sorry for the rework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted...
🎊 +1 overall
This message was automatically generated. |
Do we have any blockers to get this merged? |
Seems like no more blockers... @mukund-thakur if this looks good, requesting you to please merge this. This is urgently required for a couple of high priority scenarios... Thanks |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
@anujmodi2021 could you please create a backport pr on top of branch-3.4 and re tetst and then we can merge that as well? Thanks |
… Identity (apache#6787) Add support for Azure Active Directory (Azure AD) workload identities which integrate with the Kubernetes's native capabilities to federate with any external identity provider. Contributed By: Anuj Modi
Description of PR
Jira: https://issues.apache.org/jira/browse/HADOOP-18610
Code Ported from PR: #5953
Please refer to the original PR Description below. This PR take forward the work done by original author @creste.
Original PR Description
Add support for Azure Active Directory (Azure AD) workload identities which integrate with the Kubernetes's native capabilities to federate with any external identity provider.
This PR is based on Haifeng Chen's patch attached to HADOOP-18610. I fixed a few typos and linter errors but did not modify the core functionality.
How was this patch tested?
:::: AGGREGATED TEST RESULT ::::
============================================================
HNS-OAuth
[WARNING] Tests run: 142, Failures: 0, Errors: 0, Skipped: 2
[WARNING] Tests run: 617, Failures: 0, Errors: 0, Skipped: 73
[WARNING] Tests run: 380, Failures: 0, Errors: 0, Skipped: 54
============================================================
HNS-SharedKey
[WARNING] Tests run: 142, Failures: 0, Errors: 0, Skipped: 3
[WARNING] Tests run: 617, Failures: 0, Errors: 0, Skipped: 28
[WARNING] Tests run: 380, Failures: 0, Errors: 0, Skipped: 41
============================================================
NonHNS-SharedKey
[WARNING] Tests run: 142, Failures: 0, Errors: 0, Skipped: 9
[WARNING] Tests run: 601, Failures: 0, Errors: 0, Skipped: 268
[WARNING] Tests run: 380, Failures: 0, Errors: 0, Skipped: 44
============================================================
AppendBlob-HNS-OAuth
[WARNING] Tests run: 142, Failures: 0, Errors: 0, Skipped: 2
[WARNING] Tests run: 617, Failures: 0, Errors: 0, Skipped: 75
[WARNING] Tests run: 380, Failures: 0, Errors: 0, Skipped: 78
Time taken: 57 mins 20 secs.